-
Notifications
You must be signed in to change notification settings - Fork 8
typesync improvements #295
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
onChange={(e) => { | ||
createAppForm.setFieldValue( | ||
'directory', | ||
`./${pipe(e.target.value, EffectString.toLowerCase, EffectString.replaceAll(/\s/g, '-'))}`, | ||
); | ||
}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use the listeners
object on the createAppForm.AppField
? It has an onChange
as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no objection to use listener
sadly I'm not familiar with the TypeSync codebase nor TanStack Forms. Is listeners recommended by Tanstack forms or are there other benefits?
return [importStatement, typeDefinitions].join('\n\n'); | ||
} | ||
|
||
export function buildMappingFile(schema: Domain.InsertAppSchema) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many of the properties won't have knowledgeGraphId
if it is a custom property. So I think we should filter out the properties where property.knowledgeGraphId
is null, yeah?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spot on, created an issue here #301
can you pick it up? otherwise I do it as soon as I resolved the other urgent tasks
No description provided.